feat: add HTTP proxy support via environment variables#423
feat: add HTTP proxy support via environment variables#423femto wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9bf3701 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
/gemini review |
|
@google-cla check |
|
hello, the cla bot seemed to check co-author claude code noreply@anthropic.com , |
|
u can ask claude to remove coauthor submit if you want and then remove that previous commit , so then it wouldn't have cla error |
femto
force-pushed
the
feat/proxy-support
branch
from
3758274 to
d65de32
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces HTTP proxy support by leveraging reqwest for token refresh and the initial login flow when proxy environment variables are set. The implementation correctly addresses the problem described. My review identifies several opportunities to enhance maintainability and robustness. Specifically, I've pointed out duplicated code for proxy detection. I've also suggested improvements to error handling to prevent potential panics and a refactoring to simplify a complex conditional block. Please see the specific comments for details.
When http_proxy/https_proxy/all_proxy environment variables are set, use reqwest (which natively supports proxy) for token refresh instead of yup-oauth2's hyper-based client (which doesn't support proxy). This enables gws to work in environments that require HTTP proxy to access Google APIs (e.g., users in China). Changes: - Cargo.toml: Enable reqwest's default features including proxy support - src/auth.rs: Add proxy-aware token refresh using reqwest as fallback Fixes googleworkspace#422
When proxy env vars are set, use a custom OAuth flow with reqwest for token exchange instead of yup-oauth2's hyper-based client. Changes to auth_commands.rs: - Add login_with_proxy_support() for proxy-aware OAuth login - Add exchange_code_with_reqwest() for token exchange via reqwest - Detect proxy env vars and choose appropriate flow
femto
force-pushed
the
feat/proxy-support
branch
from
d65de32 to
9bf3701
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces HTTP proxy support for token refresh and the initial authentication flow. When proxy-related environment variables are detected, reqwest is used for network requests instead of yup-oauth2's default hyper-based client, which lacks proxy support. The changes look solid and well-tested.
3 participants
Summary
When
http_proxy/https_proxy/all_proxyenvironment variables are set, usereqwest(which natively supports proxy) for token refresh instead ofyup-oauth2'shyper-based client (which doesn't support proxy).This enables
gwsto work in environments that require HTTP proxy to access Google APIs (e.g., users in China).Changes
reqwest's default features including proxy and SOCKS supporthas_proxy_env()helper to detect proxy environment variablesrefresh_token_with_reqwest()for proxy-aware token refreshreqwestdirectly instead of waiting foryup-oauth2's hyper client to timeoutTesting
Tested in an environment requiring HTTP proxy:
Without this change, the command would hang waiting for hyper's connection timeout, then fail.
Fixes #422